Refactor Report.js to remove Ion.get() by tgolen · Pull Request #403 · Expensify/App

tgolen · 2020-09-04T04:11:29Z

I know this is a huge PR that touches a lot of things, but most of them are pretty superficial. This PR contains:

Replaces all references to "report history" with "report actions" (this is the bulk of superficial changes)
Removes all Ion.get() references in Report.js
Greatly simplifies all promise chains in Report.js to reflect the beauty of Ion being fully asynchronous (no need to do so many things as a sequence of promises)

Tests

Need to do general regression testing on pretty much everything. Be sure to test the following flow:

User A is in the chat app
Create a new user B on the same policy as user A
In a separate browser, sign into the chat app with User B
Have user B use the chat switcher to start a conversation with User A
Verify that the chat shows up for User A
Verify that notifications work
Verify that the unread indicator works

cc @quinthar

… comes in

tgolen · 2020-09-04T04:12:30Z

 */
 export default {
-    ACTIVE_CLIENT_IDS: 'activeClientIDs',
+    ACTIVE_CLIENTS: 'active_clients',


The renaming of these keys is OK. If you want to have a "clean" local storage (with no unused keys), just sign out and sign back in

cead22 · 2020-09-04T15:54:14Z

        return (
            <>
-                {_.map(this.props.reports, report => (
+                {_.map(this.props.reports, report => report.reportID && (


When can reportID not be set? Might be worth a comment

Yeah, that's a good call. This might have only been something that happened in the middle of my refactoring so I will see if this can be removed.

cead22 · 2020-09-04T15:59:52Z


+    componentDidUpdate(prevProps) {
+        // When the number of actions change, wait three seconds, then record the max action
+        // This will make the unread indicator go away if you receive comments in the same chat you're looking at


Just so I understand, if I'm in a chat and a new message comes in, the channel name will be bold, and 3 seconds later it'll the channel name will no longer be bold?

Yes, correct. Do you think that is good? The alternatives are:

It always stays bold

It doesn't turn bold at all

The delay is changed

Some other action makes it not bold anymore

I had to add a little bit of logic to this so that it will only run this if you're currently looking at the report (or else if a comment comes in on a report you aren't looking at, it disappears from the sidebar after 3 seconds 😂 )

I don't think we need this, but I think this is ok for now. I say we don't need this because I'm guessing we're going to implement a marker like slack's (screenshot below) that denotes were the first unread message is (imo, not having this is a blocker for moving group convos to expensify, because it's very hard to know where you left off) and that will make this "sidebard unread indicator" unnecessary

cead22 · 2020-09-04T16:00:32Z

    /**
     * This function is triggered from the ref callback for the scrollview. That way it can be scrolled once all the
-     * items have been rendered. If the number of items in our history have changed since it was last rendered, then
+     * items have been rendered. If the number of actions have changed since it was last rendered, then


has changed

cead22 · 2020-09-04T16:03:20Z

 import ChatSwitcherList from './ChatSwitcherList';
 import ChatSwitcherSearchForm from './ChatSwitcherSearchForm';
-import {fetchChatReport} from '../../../lib/actions/Report';
+import {fetchOrCreateChatReport} from '../../../lib/actions/Report';


getChatReport is better imo

cead22 · 2020-09-04T16:03:52Z

 };

 const SidebarLink = (props) => {
+    if (!props.reportName) {


When will a report not have a name? might be worth a comment

cead22 · 2020-09-04T16:17:56Z

- * Get the chat report ID, and then the history, for a chat report for a specific
+ * Get the actions of a report
+ *
+ * @param {string} reportID


Shouldn't this be Number?

cead22 · 2020-09-04T16:18:43Z

    let reportID;

    // Get the current users accountID and set it aside in a local variable
    // which is used for checking if there are unread comments


cead22 · 2020-09-04T16:19:48Z

 * set of participants
 *
 * @param {string[]} participants
 * @returns {Promise}


NAB * @returns {Promise} resolves with reportID

cead22 · 2020-09-04T16:22:40Z

+ * @param {object} report
+ */
+function handleReportChanged(report) {
+    // Nothing can be done without a report ID and it's OK to fail gracefully


Add why/when this would happen

cead22 · 2020-09-04T16:23:42Z

+        return;
+    }
+
+    if (report && report.reportName === undefined) {


The first condition will always be true, or the if above will fail

NAB, use _.isUndefined, or !report.reportName

Just FYI, the underscore source says:

/** @deprecated since v4.0.0 - use `value === undefined` instead. */

I also don't want to do a falsy check because an empty string should be OK (as far as not having any code throw errors anywhere).

quinthar · 2020-09-04T17:53:46Z

-    REPORT_ACTION: 'reportAction',
-    REPORT_HISTORY: 'report_history',
-    FIRST_REPORT_ID: 'first_report_id',
+    REPORT_ACTIONS: 'report_actions',


Can we all it reportActions? Basically let's just copy the database as near as possible. No need to switch from camelcase to underscore-separated.

quinthar · 2020-09-04T17:54:22Z

+    FIRST_REPORT_ID: 'first_report_id',
    MY_PERSONAL_DETAILS: 'my_personal_details',
    NETWORK: 'network',
    PERSONAL_DETAILS: 'personal_details',


Is this what the NVP is called in the database? If not, can we rename it? Indeed, for all of these, can we use camelcase to match the database and NVP nomenclature?

Sure, I'll rename them to be camel case

Jag96

Just a couple nabs, other than the points found by @cead22 this LGTM.

Jag96 · 2020-09-04T18:11:12Z

+            _.each(fetchedReports, (report) => {
                // Store only the absolute bare minimum of data in Ion because space is limited
-                const newReport = getSimplifiedReportObject(report, accountID);
+                const newReport = getSimplifiedReportObject(report);


nab - unnecessary const

Jag96 · 2020-09-04T18:16:28Z

+function fetchActions(reportID) {
+    return queueRequest('Report_GetHistory', {
+        reportID,
+        offset: 0,


nab - default offset is 0

tgolen · 2020-09-04T19:12:39Z

Thanks for the reviews, everyone! This has been updated to address everything.

Jag96

LGTM

cead22 · 2020-09-08T20:19:40Z

+    callback: val => currentURL = val,
+});
+
+// Use a regex pattern here for an exact match so it doesn't also match "my_personal_details"


"my_personal_details" => "myPersonalDetails", although probably better to update this to just say // Use a regex pattern here for an exact match

cead22 · 2020-09-08T20:27:38Z

+    if (newMaxSequenceNumber > previousMaxSequenceNumber) {
+        Ion.merge(`${IONKEYS.REPORT}_${reportID}`, {
+            hasUnread: true,
+            maxSequenceNumber: newMaxSequenceNumber,


This is the same value that was set in the call to Ion.merge above, so maybe we can condense these two calls into one

Ion.merge(`${IONKEYS.REPORT}_${reportID}`, { reportID, maxSequenceNumber: reportAction.sequenceNumber, hasUnread: reportAction.sequenceNumber > reportMaxSequenceNumbers[reportID] });

cead22 · 2020-09-08T20:30:12Z

 */
-function addHistoryItem(reportID, reportComment) {
-    const historyKey = `${IONKEYS.REPORT_HISTORY}_${reportID}`;
+function addAction(reportID, text) {


reportComment is a way better name than text. Not sure if you changed this to match the database, but text doesn't really match anything, and reportComment is a common term we use (plus the action for these is ADDCOMMENT to there is a similarity in the names)

tgolen added 23 commits September 2, 2020 10:54

Remove more get() from report action

6e14843

Remove some more instances of Ion.get()

b7d4b65

Remove final instances of Ion.get()

d763455

Switch this to merge

97e4a8d

Connect to both keys at once

e180e6b

Merge branch 'master' into tgolen-remove-get-report

3c2e094

Correcting file after merge conflicts

06bfe48

Only keep track of the reportIDs that are known

9f3f432

Only keep track of reportIDs

de54b86

Properly handle missing report names

ceed33f

Remove unnecessary prop for reportID

01d273d

Handle unknown reports better

9acad6a

Rename some of the ion keys so they follow conventions

6de9289

Rename from history to action

5494c53

Rename history to actions

965f293

history renamed to action

0982132

Final round of renaming

ee9d9c1

Remove currentReportActions and simplify promises

bf89214

Make the unread flag clear if you're looking at a chat when a comment…

59911d9

… comes in

Simplify more promise chains

2711ff3

Merge branch 'master' into tgolen-remove-get-report

2269a27

Correct merge issues and fix unread chats

b166e5b

Remove console log

71bb62c

tgolen requested review from Jag96 and cead22 September 4, 2020 04:11

tgolen self-assigned this Sep 4, 2020

tgolen added 2 commits September 3, 2020 22:20

Remove unnecessary param and simplify the ion callback

efe4191

Remove unnecessary comment

a65eecb

tgolen commented Sep 4, 2020

View reviewed changes

Fix bad variables

3441d79

cead22 requested changes Sep 4, 2020

View reviewed changes

quinthar reviewed Sep 4, 2020

View reviewed changes

Jag96 reviewed Sep 4, 2020

View reviewed changes

tgolen added 6 commits September 4, 2020 12:43

camelcase the keys

884f9ba

remove unnecessary variable and offset

5e2a96c

Fix some unread bugs

7fc2e7e

Fixing several things from PR review

f315341

Merge branch 'master' into tgolen-remove-get-report

615503f

Remove a history refernce

68871a2

tgolen added 2 commits September 4, 2020 14:01

Adding debug info for notifications

68596da

Shorten line

d11b496

Jag96 approved these changes Sep 4, 2020

View reviewed changes

cead22 approved these changes Sep 8, 2020

View reviewed changes

Merge branch 'master' into tgolen-remove-get-report

d9f3934

tgolen requested a review from AndrewGable as a code owner September 8, 2020 21:57

cead22 merged commit be446dc into master Sep 8, 2020

cead22 deleted the tgolen-remove-get-report branch September 8, 2020 22:00

Conversation

tgolen commented Sep 4, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Tests

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Jag96 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tgolen commented Sep 4, 2020

Uh oh!

Jag96 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

tgolen commented Sep 4, 2020 •

edited

Loading